Skip to content

Fix issue with sync watermark divergence - #5

Merged
aron-cf merged 2 commits into
mainfrom
fix-sync
Jun 15, 2026
Merged

Fix issue with sync watermark divergence#5
aron-cf merged 2 commits into
mainfrom
fix-sync

Conversation

@aron-cf

@aron-cf aron-cf commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Container-side filesystem writes were not making it back to the durable object after the first exec. A burst of printf > /workspace/foo.txt calls each succeeded inside the container but the host's ws.fs.readFile returned ENOENT for every file after the first. Once a workspace fell into this state, subsequent execs could not pull the missing entries even though the bytes were in wsd's store.

The root cause was a loopback-suppression optimization inside applyChanges. After applying an upstream batch the apply path advanced the local pushRev to the post-apply currentRev, moving it past entries the container had no way to know we had shipped. The container's fetchRev stayed where the last real push put it. The cross-side invariant check inside pullOnce then tripped on the next pull, the exec bracket swallowed the error, and fetchRev could no longer advance.

sequenceDiagram
  participant DO
  participant Container
  Note over DO,Container: before — pushRev advanced locally
  DO->>Container: push (senderRev=N)
  Container-->>DO: appliedPushRev=N
  Note over DO: applyChanges (upstream)<br/>local rev bumps to N+1<br/>pushRev = N+1 (local advance)
  DO->>Container: fetchChanges
  Container-->>DO: appliedPushRev=N, currentRev=N+1
  Note over DO: assert N >= N+1 — fails<br/>caught silently, fetchRev frozen

  Note over DO,Container: after — pushRev stays put
  DO->>Container: push (senderRev=N)
  Container-->>DO: appliedPushRev=N
  Note over DO: applyChanges (upstream)<br/>local rev bumps to N+1<br/>pushRev stays at N
  DO->>Container: push (senderRev=N+1, redundant entries)
  Note over Container: alreadyApplied drops them<br/>fetchRev = N+1
  Container-->>DO: appliedPushRev=N+1
  DO->>Container: fetchChanges
  Container-->>DO: appliedPushRev=N+1, currentRev=N+2
  Note over DO: assert N+1 >= N+1 — passes
Loading

The fix has two parts. Stop advancing pushRev locally on upstream apply: the receiver's alreadyApplied check drops the redundant entries on the next pushOnce and the container's fetchRev catches up in the same round trip. The cost is one extra push per upstream apply, bounded by the batch. Make pullOnce recover inline when a divergence does occur: cancel the stream, reset the divergent cursor to zero, retry once. A second divergence after the retry still surfaces via the existing assertion.

Unit tests cover both halves. The dofs apply suite pins the new contract: pushRev stays put on upstream apply, the next coalesce surfaces the apply's rev bumps, and the receiver's idempotence does the rest. The rpc suite pins the inline recovery: a one-shot lying remote triggers a reset and the retried call drains normally; a persistently lying remote degrades to a baseline re-sync rather than throwing.

@aron-cf
aron-cf marked this pull request as ready for review June 15, 2026 14:45
aron-cf added 2 commits June 15, 2026 19:32
The loopback-suppression optimization in applyChanges advanced the
local pushRev to currentRev after every upstream apply, on the
theory that the apply's own rev bumps would otherwise get re-pushed.
That theory was correct in isolation but the implementation was
unsound: it moved our pushRev past entries the remote did not know
we had shipped, while the remote's fetchRev (echoed back as
appliedPushRev on every fetchChanges response) stayed where the
last real push had put it. The cross-side invariant check in
pullOnce then trips on the very next pull and the post-drain pull
in the exec bracket swallows the error, leaving every subsequent
container-side write invisible to the host until something
reconciles the watermarks.

Drop the local advance. The next pushOnce ships the apply's rev
bumps, the receiver's alreadyApplied() check drops them as no-ops,
and the container's fetchRev catches up to our pushRev in the same
round trip. One extra push per upstream apply, bounded by the
batch's coalesce output. The cross-side invariant stays intact.

The two F1 tests on this behavior already covered the unsafe case
(do not strand unpushed locals); they still pass. The two tests
that pinned the optimization (one in apply.test.ts, one in
wire.test.ts) are updated to assert the new contract.
reconcileWatermarks runs once per connect and resets local cursors
to 0 when the remote is behind us. That handles the WebSocket-drop
case but not the WebSocket-survives-wsd-restart case: wsd's store is
process-lifetime, so a wsd respawn under the same WS leaves the DO
holding cursors the container no longer knows about, and the next
pull's cross-side invariant assertion throws.

Move the recovery inline. When fetchChanges reports an
appliedPushRev below our localPushRev, or a currentRev below our
fetchRev, treat it as a real-time reconcile: cancel the in-flight
stream, reset the divergent cursor to 0, and recurse once. The
rev-0 baseline path re-ships incrementally and the receiver's
alreadyApplied() check absorbs the work. A second divergence after
the retry surfaces via the existing assertion, so a persistently
broken remote still fails loudly.

Combined with the prior pushRev-locality fix this closes the
'FUSE write invisible to DO readFile' bug observed on the deployed
container example: the apply-side fix prevents the divergence from
being introduced, and this inline recovery prevents any future
divergence (mid-flight restart, harness shenanigans, ...) from
wedging the same way.
@aron-cf
aron-cf merged commit f9e2ec4 into main Jun 15, 2026
9 checks passed
@aron-cf
aron-cf deleted the fix-sync branch June 15, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant